Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DataPipe] Adding a 's' to the functional names of open/list DataPipes #479

Closed
wants to merge 3 commits into from

Conversation

NivekT
Copy link
Contributor

@NivekT NivekT commented May 31, 2022

NivekT added a commit that referenced this pull request May 31, 2022
ghstack-source-id: 3bb1bce0576bc0ef5c298bf7bbc85b88bc8d4945
Pull Request resolved: #479
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 31, 2022
Copy link
Contributor

@ejguan ejguan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this PR. Could you please also fix tests?

@NivekT
Copy link
Contributor Author

NivekT commented May 31, 2022

Thank you for adding this PR. Could you please also fix tests?

I don't think there are any existing use cases in tests but I have added some.

NivekT added a commit that referenced this pull request May 31, 2022
ghstack-source-id: c2d9ee08447c04e6a116943b5fb03b70edf1c722
Pull Request resolved: #479
NivekT added a commit that referenced this pull request May 31, 2022
ghstack-source-id: 20d002fe1a679e4795f2afd7016dd0a22497c7d5
Pull Request resolved: #479
Comment on lines +136 to +137
# Register for functional API for backward compatibility
IterDataPipe.register_datapipe_as_function("open_file_by_fsspec", FSSpecFileOpenerIterDataPipe)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up task, add deprecation warning for the functional API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we will need to change the registration function and IterDataPipe class in PyTorch core to do that. One possibility is described here.

Do you think we should land this PR as it is or make the changes to core first?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's land this PR first and change the core afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, could you please update the deprecation table as well?

@NivekT
Copy link
Contributor Author

NivekT commented May 31, 2022

@NivekT has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@NivekT NivekT mentioned this pull request May 31, 2022
NivekT added a commit that referenced this pull request Jun 1, 2022
Summary: Pull Request resolved: #479

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D36785643

Pulled By: NivekT

fbshipit-source-id: 02c3071047ac00dd34cb83a9b392be0cfa3565b0
NivekT added a commit that referenced this pull request Jun 1, 2022
)

Summary: Pull Request resolved: #479

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D36785643

Pulled By: NivekT

fbshipit-source-id: 02c3071047ac00dd34cb83a9b392be0cfa3565b0
bushshrub pushed a commit to bushshrub/data that referenced this pull request Jun 2, 2022
)

Summary: Pull Request resolved: pytorch#479

Test Plan: Imported from OSS

Reviewed By: ejguan

Differential Revision: D36785643

Pulled By: NivekT

fbshipit-source-id: 02c3071047ac00dd34cb83a9b392be0cfa3565b0
@facebook-github-bot facebook-github-bot deleted the gh/NivekT/81/head branch June 4, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants